Hi Vedanth - Thanks for your PR! Your architectural direction is sound -- moving expensive database-backed cache warmup from every API request to application startup is a real performance win, and the handler refactoring is clean and consistent. However, there are issues in the test infrastructure changes and a few production code concerns that should be addressed before we can get this merged.

Critical

1. Potential nil dereference if gtfsManager is nil (cmd/api/app.go)

Lines 49-57: The directionCalculator creation is guarded by if gtfsManager != nil, but the InitializeGlobalCache call on line 54 dereferences gtfsManager.GtfsDB.Queries unconditionally. If gtfsManager were nil, this would panic.

var directionCalculator *gtfs.AdvancedDirectionCalculator
if gtfsManager != nil {
    directionCalculator = gtfs.NewAdvancedDirectionCalculator(gtfsManager.GtfsDB.Queries)
}

err = gtfs.InitializeGlobalCache(context.Background(), gtfsManager.GtfsDB.Queries, directionCalculator)

Fix: Move the InitializeGlobalCache call inside the if gtfsManager != nil block.

2. Generated code (query.sql.go) diverges from source SQL (query.sql)

The source query.sql still uses MATCH @query / ORDER BY bm25(routes_fts), but the generated query.sql.go now has MATCH ?1 / ORDER BY rank. The PR body acknowledges this as a known SQLite FTS5 & SQLC conflict, but the divergence means running make models will produce different output than what's checked in. This violates the project convention that generated code should match what make models produces.

Fix: Either update query.sql to match and regenerate, or confirm this is the exact output of make models with the current sqlc version and note it in the PR description.

Important

3. InitializeGlobalCache called on every test run, error silently discarded (http_test.go:74)

directionCalculator := gtfs.NewAdvancedDirectionCalculator(testGtfsManager.GtfsDB.Queries)
_ = gtfs.InitializeGlobalCache(context.Background(), testGtfsManager.GtfsDB.Queries, directionCalculator)

Two issues here:

• Error discarded: If cache warmup fails, tests proceed silently with an unwarmed cache. In production, this same failure prevents the app from starting.
• Repeated work: This runs on every call to createTestApiWithClock, creating a new calculator and re-running the full cache warmup each time. The GTFS manager is properly inside sync.Once, but these lines are not.

Fix: Move directionCalculator creation and InitializeGlobalCache inside testDbSetupOnce.Do(...), and check the error with require.NoError.

4. Dead dataPath fields in table-driven tests (gtfs_manager_test.go)

After the refactor, TestManager_GetAgencies, TestManager_RoutesForAgencyID, and TestManager_GetStopsForLocation_UsesSpatialIndex still define dataPath in their test case structs but never use it -- all tests call getSharedTestComponents(t) which ignores the path. This is misleading dead code.

Fix: Either remove the table-driven structure (since there's now only one data source), or simplify the test case structs.

5. TestManager_GetVehicleForTrip mutates shared manager state (gtfs_manager_test.go:311-333)

This test writes directly to manager.realTimeVehicles on the shared singleton manager. Since sharedManager is reused across all tests, this mutation is visible to every subsequent test. If test execution order changes or parallelism is introduced, this creates a data race.

Fix: This test should create its own isolated manager instance since it modifies mutable state.

6. processRouteStops accepts unused adc parameter (stops_for_route_handler.go:108)

The function accepts adc *GTFS.AdvancedDirectionCalculator but never uses it -- buildStopsList accesses api.DirectionCalculator directly. This dead parameter was left behind during the refactoring.

Fix: Remove the adc parameter from processRouteStops.

Fit and Finish

7. ShapePtSequence dropped in shape cache mapping (global_cache.go:58-64)

The mapping from Shape to GetShapePointsWithDistanceRow omits ShapePtSequence (defaults to 0). While calculateOrientationAtStop doesn't use this field (it relies on slice ordering and ShapeDistTraveled), the existing direction_precomputer.go:271 and shape_cache_test.go:223 do preserve/check it. Including it would be more consistent and future-proof.

8. DISTINCT unnecessary in GetAllStopIDs query

SELECT DISTINCT id FROM stops -- id is the primary key, so DISTINCT is redundant. Harmless but unnecessary.

9. Misleading slog.Warn before return err (global_cache.go:53-55)

Using Warn level then immediately returning a fatal error is contradictory. The caller in app.go treats this as a startup failure. Either remove the log (the caller already wraps the error) or use slog.Error. The indecisive comment on line 52 ("Fail fast if we can't load shapes (or just log error if you want to be resilient)") should also be removed.

10. No Shutdown() on shared test manager

The sync.Once singleton sharedManager in advanced_direction_calculator_test.go never calls Shutdown(). While benign for in-memory SQLite (process exits), it's inconsistent with the project's cleanup patterns. A TestMain function would handle this cleanly.

11. RoutesFt struct added to models.go but unused

This appears to be an artifact of sqlc auto-generating a model for the routes_fts FTS5 virtual table. If it came from make models, it's fine (just generated noise). If manually added, it should be removed.

12. Error paths in InitializeGlobalCache lack wrapping context

The three error returns in global_cache.go return bare errors from different queries (GetAllStopIDs, GetStopsWithShapeContextByIDs, GetShapePointsByIDs). When these propagate up, operators can't distinguish which query failed without reading source code.

Fix: Wrap each with fmt.Errorf("failed to fetch X: %w", err).

Pre-existing (not blocking, but noted)

• schedule_for_route_handler.go:244-247: BuildStopReferencesAndRouteIDsForStops errors are silently swallowed with if err == nil. This predates the PR but is worth flagging since the PR modifies this call site.

The architectural approach is good and the handler refactoring is clean, but the items noted here will need to be addressed before we can get this merged. Thanks for your submission and I look forward to getting it merged!

Hey Vedanth -- Excellent work addressing the feedback from the first round. All 12 items have been properly resolved, and the architectural approach is clean. The handler refactoring is consistent, the InitializeGlobalCache error handling is solid with proper fmt.Errorf wrapping, and the test singleton pattern is thoughtful. This is looking very close to merge-ready. Just a few remaining items to tidy up.

Prior Feedback Status

All 12 items from the first review have been addressed:

Important

1. createTestApiWithRealTimeData missing DirectionCalculator (vehicles_for_agency_handler_test.go:343-352)

This function constructs an app.Application without setting DirectionCalculator. Since all handlers now use api.DirectionCalculator.CalculateStopDirection(...), any test path that reaches a direction calculation will panic with a nil pointer dereference. Tests pass today because the real-time test paths don't happen to hit the direction calculation code, but this is a latent bug -- one new test or data change away from crashing.

Fix: Add DirectionCalculator initialization to createTestApiWithRealTimeData:

dirCalc := gtfs.NewAdvancedDirectionCalculator(gtfsManager.GtfsDB.Queries)
err = gtfs.InitializeGlobalCache(context.Background(), gtfsManager.GtfsDB.Queries, dirCalc)
require.NoError(t, err)

application := &app.Application{
    // ...existing fields...
    DirectionCalculator: dirCalc,
}

2. TestManager_GetVehicleForTrip missing defer manager.Shutdown() (gtfs_manager_test.go:301)

This test correctly creates its own isolated manager (since it mutates realTimeVehicles), but doesn't clean it up. Add defer manager.Shutdown() after the nil check. Also worth adding a brief comment explaining why this test doesn't use getSharedTestComponents -- future contributors might wonder.

3. Vestigial single-element table-driven tests (gtfs_manager_test.go)

TestManager_GetAgencies and TestManager_RoutesForAgencyID still use table-driven structure with a single test case whose only field is name. The dataPath field that justified the table was removed, but the scaffolding remains. Simplify to direct tests:

func TestManager_GetAgencies(t *testing.T) {
    manager, _ := getSharedTestComponents(t)
    assert.NotNil(t, manager)

    agencies := manager.GetAgencies()
    assert.Equal(t, 1, len(agencies))
    // ...
}

Fit and Finish

4. tc := tc unnecessary in Go 1.24.2 (gtfs_manager_test.go:94,280)

Since Go 1.22, loop variables are scoped per-iteration. These captures are dead code. Remove them.

5. No-op int64() cast (global_cache.go:62)

GetShapePointsByIDsRow.ShapePtSequence is already int64, so int64(p.ShapePtSequence) is a no-op cast. The comment // Added ShapePtSequence is also unclear. Simplify to just ShapePtSequence: p.ShapePtSequence.

6. Noise comments in global_cache.go

Comments like // Map the DB row to the Cache row struct, // Set Cache, and // Fetch ALL Stop IDs describe what the next line obviously does. The code is clear without them. Consider removing or replacing with comments that explain why.

Pre-existing (not blocking, but noted)

• schedule_for_route_handler.go:193,219: Two if err == nil patterns for GetAgency and GetTripsByIDs silently swallow errors. The PR correctly fixed the BuildStopReferencesAndRouteIDsForStops call on line 244, but these two remain. Worth addressing in a follow-up.

Nice job systematically addressing all the first-round feedback, Vedanth. The important items above are straightforward fixes. Once those are in, this is good to go.

Hey Vedanth -- Systematic and thorough work addressing every item from both prior rounds. All 6 items from the second review are properly resolved, and the schedule_for_route_handler.go error handling fix goes beyond what was required by also fixing the two pre-existing if err == nil patterns I noted as "not blocking." That's great initiative. Just one handler that was missed in the refactor and a couple of smaller items.

Prior Feedback Status

All 6 items from the second review have been addressed:

Bonus: The pre-existing if err == nil patterns in schedule_for_route_handler.go for GetAgency (line 184) and GetTripsByIDs (line 213) have also been fixed with proper if err != nil error handling. Nice catch.

Important

1. stops_for_location_handler.go not migrated to shared DirectionCalculator (stops_for_location_handler.go:203)

This handler still creates a per-request calculator:

calc := gtfs.NewAdvancedDirectionCalculator(api.GtfsManager.GtfsDB.Queries)

Every other handler was refactored to use api.DirectionCalculator, but this one was missed. The fresh calculator won't have the global cache populated, so it falls back to per-stop database queries on every request -- defeating the purpose of the global cache warmup for this endpoint.

Fix: Replace with api.DirectionCalculator.CalculateStopDirection(...), following the same pattern as the other handlers.

2. No dedicated unit tests for InitializeGlobalCache (global_cache.go)

The function is exercised indirectly via getSharedTestComponents() and createTestApiWithClock(), but there's no global_cache_test.go that directly tests:

• Error paths (what happens when GetAllStopIDs, GetStopsWithShapeContextByIDs, or GetShapePointsByIDs fail)
• Empty database (0 stops)
• Stops with no associated shapes (len(uniqueShapeIDs) == 0 branch)

Since this function is the sole mechanism for populating the direction caches in production, it deserves direct test coverage.

Fit and Finish

3. Fetch Context (Stop -> Shape mappings) comment (global_cache.go:19)

This comment is borderline -- it explains what "context rows" means, which is useful. But the // Fetch Shape Points (Geometry) comment on line 48 follows the same "what, not why" pattern from the previous round. Consider removing line 48 or rephrasing to explain why shape points are needed separately from context rows.

4. Blank line at top of TestManager_GetAgencies and TestManager_RoutesForAgencyID (gtfs_manager_test.go:18,38)

Both functions have a leading blank line before the first statement. Minor formatting nit.

Pre-existing (not blocking, but noted)

• schedule_for_route_handler.go:138-139,146-147: Silent continue statements in the inner loop for GetOrderedStopIDsForTrip and GetStopTimesForTrip -- database errors are silently swallowed with no logging. Worth a follow-up.
• stops_for_route_handler.go:288-289,303-304: Same silent continue pattern in processTripGroups.

The missed handler in item 1 is the key remaining fix. Once that's in and global_cache_test.go is added, this is good to merge.